Validate towhat policy field #18032

jvlcek · 2018-09-28T13:29:11Z

https://bugzilla.redhat.com/show_bug.cgi?id=1435780

Invalid values for policy field towhat are not recognized, result in the policy being
created using the API.

The validations for mode were added in [PR 14519](#14519

The list of valid values for towhat were arrived at by examination of what is supported via
the UI tree_builder_policy.rb

Steps for Testing/QA

Attempt to create a policy with the following data:

    {
      "action": "create",  
      "resources": [{
        "name": "test policy bobsyouruncle",
        "description": "Desc bobsyouruncle",
        "mode": "control",
        "towhat": "bobsyouruncle",      
        "conditions_ids": [ 2000, 3000 ],
        "policy_contents": [{             
          "event_id": 2,
          "actions": [ {"action_id": 1, "opts": { "qualifier": "failure" } } ]
        }]
      }]
    }

e.g.:

$MY_MIQ_REPO/manageiq/tools/rest_api.rb  -l https://10.8.99.50 get /api/policies

<when prompted enter>

        {
          "action": "create",  
          "resources": [{
            "name": "test policy bobsyouruncle",
            "description": "Desc bobsyouruncle",
            "mode": "control",
            "towhat": "bobsyouruncle",      
            "conditions_ids": [ 2000, 3000 ],
            "policy_contents": [{             
              "event_id": 2,
              "actions": [ {"action_id": 1, "opts": { "qualifier": "failure" } } ]
            }]
          }]
        }

https://bugzilla.redhat.com/show_bug.cgi?id=1435780

jvlcek · 2018-09-28T13:29:56Z

@miq-bot add_label bug

jvlcek · 2018-09-28T13:30:04Z

@miq-bot assign @bdunne

jvlcek · 2018-09-28T13:30:39Z

@miq-bot add_label hammer/yes

bdunne · 2018-09-28T13:41:34Z

app/models/miq_policy.rb

@@ -1,6 +1,16 @@
 # TODO: Import/Export support

 class MiqPolicy < ApplicationRecord
+  TOWHAT_APPLIES_TO_CLASSES = %w(Host


Can this be sorted alphabetically?

👍 will do

bdunne · 2018-09-28T15:30:19Z

spec/models/miq_policy_spec.rb

+
+  context '.validates' do
+    it 'validates towhat and mode' do
+      expect(described_class.create!(:towhat      => "Host",


You don't need to save the record to test this, you can

expect(FactoryGirl.build(:miq_policy, :towhat => "Host", :mode => "compliance").valid?).to be true

Should mode even be tested here since it didn't change in this PR?

I'm usually not a huge fan of this syntax, but you could also

expect(FactoryGirl.build(:miq_policy, :towhat => "Host")).to be_valid

@bdunne Thank you for the input. I'll have the new test focus on towhat

I like the syntax the way it is and if you don't have strong feelings about it I'd like to leave it as is, mostly because it's more consistent with the tests through the spec.

Let me know if you are OK with leaving the test as is.

Thank you. JoeV

I guess I don't see the point of writing to the database if we just want to make sure the instance is valid. Also, using FactoryGirl makes it much more obvious that you're only testing :towhat and not any other validation.

I guess the question is: are you testing that it's valid or testing if it raises an exception. If you're testing validity: you don't need to create the record, you can do as @bdunne suggests. If you're testing exception raising, you need to create!

It looks like your testing validity so create! isn't needed here. It's minor and I know other code does this but it's fine to mix patterns in the specs until you can convert the rest to the new way.

@bdunne and @jrafanie Ah I see the reasoning now. Thank you for explaining. Will do.

JoeV

yeah, I prefer expecting the specific thing (validity) rather than the side effect of rails raising an error when calling create! on an invalid instance.

bdunne · 2018-09-28T15:38:15Z

spec/models/miq_policy_spec.rb

+
+    it 'reports invalid towhat' do
+      expect do
+        described_class.create!(:towhat      => "BobsYourUncle",


Similar here

expect(FactoryGirl.build(:miq_policy, :towhat => "BobsYourUncle")).not_to be_valid

@bdunne Here again, if you feel strongly I'd be glad to change this test but for me it's much easier to quickly grasp what the test is doing, that a particular exception is expected and it's more consistent with existing tests in the spec.

Let me know if you are OK with it as is.

Thank you for the review feedback.
JoeV

I'd be ok with testing create! if you're concerned with a custom validation message but since this is out of the box validation messages, I think you can just do thing.new + thing.valid? and maybe even thing.errors[:towhat].blank? or something like that.

@jrafanie Thank you for explaining. Will do.

JoeV

jvlcek · 2018-09-28T17:20:36Z

@bdunne and @jrafanie Now that I see the result I appreciate the simplicity of this version of the code!
Thank you for your input.

JoeV

miq-bot · 2018-09-28T17:22:05Z

Checked commits jvlcek/manageiq@d344e43~...a9bdbd9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Validate towhat policy field (cherry picked from commit 5270416) https://bugzilla.redhat.com/show_bug.cgi?id=1435780

simaishi · 2018-10-01T16:58:43Z

Hammer backport details:

$ git log -1
commit 28ab0177beb315a791af3d60a7ca5ae1cfc5c29a
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Fri Sep 28 14:38:24 2018 -0400

    Merge pull request #18032 from jvlcek/bz_1435780_tow_hat_validations
    
    Validate towhat policy field
    
    (cherry picked from commit 52704166f4dc48ccc2ced8c6bac0e8001e8d786f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1435780

ManageIQ/manageiq#18032

Validate towhat policy field

d344e43

https://bugzilla.redhat.com/show_bug.cgi?id=1435780

miq-bot added the bug label Sep 28, 2018

miq-bot assigned bdunne Sep 28, 2018

miq-bot added the hammer/yes label Sep 28, 2018

bdunne requested changes Sep 28, 2018

View reviewed changes

jvlcek added 2 commits September 28, 2018 11:59

Alphabetize TOWHAT_APPLIES_TO_CLASSES and focus new tests on towhat.

6152ab4

Simplify the new specs based on review feedback.

a9bdbd9

bdunne approved these changes Sep 28, 2018

View reviewed changes

bdunne merged commit 5270416 into ManageIQ:master Sep 28, 2018

bdunne added this to the Sprint 96 Ending Oct 8, 2018 milestone Sep 28, 2018

jvlcek deleted the bz_1435780_tow_hat_validations branch October 1, 2018 11:36

simaishi pushed a commit that referenced this pull request Oct 1, 2018

Merge pull request #18032 from jvlcek/bz_1435780_tow_hat_validations

28ab017

Validate towhat policy field (cherry picked from commit 5270416) https://bugzilla.redhat.com/show_bug.cgi?id=1435780

simaishi added hammer/backported and removed hammer/yes labels Oct 1, 2018

bdunne added a commit to bdunne/manageiq-api that referenced this pull request Oct 2, 2018

Fix policy spec due to new validation

7a6c498

ManageIQ/manageiq#18032

bdunne mentioned this pull request Oct 2, 2018

Fix policy spec due to new validation ManageIQ/manageiq-api#484

Merged

This was referenced Jan 8, 2019

Clarify the 'towhat' inclusion validation message on policy creation. #18338

Merged

Update Policy creation 'towhat' field documentation ManageIQ/manageiq-documentation#1044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate towhat policy field #18032

Validate towhat policy field #18032

jvlcek commented Sep 28, 2018

jvlcek commented Sep 28, 2018

jvlcek commented Sep 28, 2018

jvlcek commented Sep 28, 2018

bdunne Sep 28, 2018

jvlcek Sep 28, 2018

bdunne Sep 28, 2018

bdunne Sep 28, 2018

bdunne Sep 28, 2018

jvlcek Sep 28, 2018

bdunne Sep 28, 2018 •

edited

Loading

jrafanie Sep 28, 2018

jvlcek Sep 28, 2018

bdunne Sep 28, 2018

bdunne Sep 28, 2018

jvlcek Sep 28, 2018

jrafanie Sep 28, 2018 •

edited

Loading

jvlcek Sep 28, 2018

jvlcek commented Sep 28, 2018

miq-bot commented Sep 28, 2018

simaishi commented Oct 1, 2018

Validate towhat policy field #18032

Validate towhat policy field #18032

Conversation

jvlcek commented Sep 28, 2018

Steps for Testing/QA

jvlcek commented Sep 28, 2018

jvlcek commented Sep 28, 2018

jvlcek commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvlcek commented Sep 28, 2018

miq-bot commented Sep 28, 2018

simaishi commented Oct 1, 2018

bdunne Sep 28, 2018 •

edited

Loading

jrafanie Sep 28, 2018 •

edited

Loading